Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle ActiveRecord::Locking::LockingType #2014

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kbruccoleri
Copy link

@kbruccoleri kbruccoleri commented Sep 6, 2024

Motivation

ActiveRecord::Locking::LockingType is used for the lock_version column to handle Optimistic Locking:

https://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html

This type was falling into the else block and causing a Sorbet Error:

Expected type ::Object, got type ActiveRecord::Locking::LockingType with value #<ActiveModel::Type::Intege...ange=-2147483648...2147483648> (SorbetError)

Experiencing an issue after upgrading to latest Tapioca for Rails 7.2 compatibility.

Implementation

The column_type is really just a wrapper for a potentially nilable integer, so we make a specific exception for it.

Tried to follow the convention around it with regards to determining if particular column is nilable or not.

Tests

No existing test file for lib/tapioca/dsl/helpers/active_record_column_type_helper.rb

Shoutout to @cquinones100 for co-authoring

@kbruccoleri
Copy link
Author

I have signed the CLA!

@amomchilov amomchilov self-assigned this Sep 12, 2024
@amomchilov amomchilov added the enhancement New feature or request label Sep 12, 2024
Copy link
Contributor

@amomchilov amomchilov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Kevin, thanks for your first contribution!

Can you please credit @cquinones100 correctly by adding this to the end of your commit description:

Co-authored-by: NAME <[email protected]>

That'll make GitHub properly show you both as authors

https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

@amomchilov
Copy link
Contributor

There's some unrelated CI failures btw. I'm working through those now.

@kbruccoleri kbruccoleri force-pushed the handle-activerecord-locking-lockingtype branch from 507f1ab to 2d5e3c4 Compare September 12, 2024 19:02
@kbruccoleri
Copy link
Author

@amomchilov Thanks for the feedback and the guidance, I have accepted all of your edits and updated the co-authors.

@amomchilov
Copy link
Contributor

amomchilov commented Sep 12, 2024

@kbruccoleri Excellent! I'll follow up on this once I've sorted out the CI issues

In the meantime, you please fix the lint failure? My hand-scribbled indentation suggestions were incorrect haha.

bundle exec rubocop --auto-correct lib/tapioca/dsl/helpers/active_record_column_type_helper.rb

@egiurleo
Copy link
Contributor

Hi @kbruccoleri! You can rebase on main, which should fix the CI issues. If you could also fix the linting issues, that would be much appreciated!

kbruccoleri and others added 2 commits September 26, 2024 10:07
ActiveRecord::Locking::LockingType is used for the lock_version column to handle Optimistic Locking:

https://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html

- This type was falling into the else block and causing a Sorbet Error:

Expected type ::Object, got type ActiveRecord::Locking::LockingType with value #<ActiveModel::Type::Intege...ange=-2147483648...2147483648> (SorbetError)

The column_type is really just a wrapper for a potentially nilable integer, so we make a specific exception for it.

Co-authored-by: Carlos Quinones <[email protected]>
Co-authored-by: Alexander Momchilov <[email protected]>
@kbruccoleri kbruccoleri force-pushed the handle-activerecord-locking-lockingtype branch from 436a670 to 46688aa Compare September 26, 2024 14:07
@kbruccoleri
Copy link
Author

Hi @kbruccoleri! You can rebase on main, which should fix the CI issues. If you could also fix the linting issues, that would be much appreciated!

@egiurleo Thanks for flagging - I believe I am rebased on main now and have bin/style passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants